Fix contact autocomplete when no legacy accounts are loaded - #2824
ejbiker93ss wants to merge 2 commits into
Conversation
bengotow
left a comment
bengotow
left a comment
There was a problem hiding this comment.
👋 Heads up: I'm Claude, an AI reviewer, running at a maintainer's request. Everything below is a suggestion for the human maintainers to weigh — please push back where you think I've got it wrong, and don't treat any of it as a merge gate.
Thanks for taking the time to dig into this and write it up — "typing support returns nothing" is a real and genuinely annoying bug, and the report is appreciated. I do have concerns about the diagnosis and the shape of the fix, though, so I've left detailed inline notes. Summarizing the main points here:
1. I don't think the LIMIT 0 diagnosis holds up
AccountStore loads accounts synchronously from config.json in its constructor (app/src/flux/stores/account-store.ts:78), not from the database, so accounts() shouldn't ever be transiently empty. It's only empty when the user has zero linked accounts — and then there are no contacts to search and no composer open. The cross-check: topContacts() does the same limit * accountCount multiplication, so if this were the cause, top contacts would be broken for everyone too.
The Math.max(accountCount, 1) guard is harmless, but I'd expect it to be a no-op for your symptom. If you're actually observing accountCount === 0, that's a serious bug worth its own issue — could you share how you confirmed it?
2. A likely real culprit, if you want to chase it
SearchMatcher.whereSQL (app/src/flux/attributes/matcher.ts:352) generates:
`Contact`.`id` IN (SELECT `content_id` FROM `ContactSearch` WHERE `ContactSearch` MATCH '"support"*' LIMIT 1000)That inner LIMIT 1000 truncates FTS hits before the refs > 0 / hidden = false filters and the ORDER BY refs DESC are applied. A generic token like support matches an enormous number of rows (every no-reply and vendor support address that ever landed in your mail), so the 1000 rows that survive the cut can easily contain none of the contacts you actually correspond with — producing exactly "nothing shows up." Meanwhile a distinctive name returns results fine, which matches the shape of the report.
That would be a much smaller, more targeted fix. Some diagnostics that would confirm or rule it out, run in the dev tools console:
// how many FTS rows match, and how many survive the filters?
$m.DatabaseStore._query(`SELECT COUNT(*) FROM ContactSearch WHERE ContactSearch MATCH '"support"*'`)
$m.DatabaseStore.findAll($m.Contact).search('support').then(r => console.log(r.length, r))If the first number is well above 1000 while the second comes back empty or irrelevant, that's the bug.
3. The PR is really two changes, and I'd suggest splitting them
Beyond the limit tweak, this adds _searchSentRecipientContacts — a new Thread FTS lookup that mines contacts out of recent threads. That's a feature, and it's not mentioned in the title. It also lands on the keystroke path without .background(), so it blocks the renderer on every character typed, over an unbounded FTS subselect. And it reorders all autocomplete results ahead of the existing refs ranking. Details are in the inline comments on contact-store.ts.
My suggestion would be:
- PR A: the minimal fix for the actual root cause, with a regression test that fails before and passes after.
- PR B (optional, separate): the thread-history fallback, with
.background(), input debouncing, a bounded FTS subselect, and an explicit decision about how it should interact withrefs-based ranking.
4. Smaller items (inline)
if (limit === 0) return []is unreachable..flat() as Contact[]asserts away a type the component genuinely depends on.key={p.id || p.email}needs a word of explanation.console.warnon the keystroke path →AppEnv.reportError.- The new specs restate their implementation; the
ContactStoresuite is stillxdescribe'd, sosearchContactshas no real coverage.
None of this is meant to discourage you — the underlying report is solid and worth fixing properly. If you can grab those console numbers, I think we can pin down the real cause quickly.
Generated by Claude Code
| export const contactSearchFetchLimit = (limit: number, accountCount: number) => | ||
| limit * Math.max(accountCount, 1); |
There was a problem hiding this comment.
This guard is reasonable defensively, but I don't think accountCount === 0 is reachable in a way that would cause the symptom you hit.
AccountStore._accounts is populated synchronously in the store's constructor from AppEnv.config.get('accounts') (app/src/flux/stores/account-store.ts:78) — it reads from config.json, not from the database, and it happens in every window before any store consumer runs. There's no asynchronous "Account row" load that could leave the array empty while contacts exist. accounts() is only empty when the user genuinely has zero linked accounts, in which case the Contact table is empty too and there's no composer to autocomplete in.
A useful cross-check: if LIMIT 0 were the cause, topContacts() would be equally broken for every user, since it does the same multiplication — and it isn't.
So I'd expect this change to be a no-op for the bug you reported. Happy to be proven wrong if you're seeing accountCount === 0 in practice — if so, could you share how you confirmed it (e.g. AccountStore.accounts() in the dev tools console at the moment autocomplete fails)? That'd be a significant bug in its own right.
Generated by Claude Code
| if (limit === 0) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
This branch is unreachable. On line 78, Math.max(options.limit ? options.limit : 5, 0) maps a passed-in 0 to 5 (because 0 is falsy), so limit can only be 0 when a caller passes a negative number. Worth dropping.
Generated by Claude Code
| _searchSentRecipientContacts(search: string, limit: number): Promise<Contact[]> { | ||
| if (search.length < 2) { | ||
| return Promise.resolve([]); | ||
| } | ||
|
|
||
| const recipientSearch = new ToQueryExpression( | ||
| new TextQueryExpression(new SearchQueryToken(search)) | ||
| ); | ||
| const threadLimit = Math.min(Math.max(limit * 20, 100), 500); | ||
|
|
||
| return DatabaseStore.findAll<Thread>(Thread) | ||
| .structuredSearch(recipientSearch) | ||
| .where(Thread.attributes.lastMessageSentTimestamp.greaterThan(new Date(0))) | ||
| .order(Thread.attributes.lastMessageSentTimestamp.descending()) | ||
| .limit(threadLimit) | ||
| .then((threads) => contactsMatchingEmailPrefix(threads, search).slice(0, limit)); | ||
| } |
There was a problem hiding this comment.
This is the part I'd most want to see split out and reworked — it's a substantial new feature rather than a limit fix, and as written I think it will cause noticeable typing lag on large mailboxes. Three specific issues:
1. It runs synchronously on the render thread, on every keystroke. TokenizingTextField._refreshCompletions is called from every input event with no debounce (app/src/components/tokenizing-text-field.tsx:847), and Mailspring uses better-sqlite3, which blocks. The existing thread search deliberately avoids this by marking the query as background — see app/internal_packages/thread-search/lib/search-query-subscription.ts:50 and search-bar-util.ts:41, both of which call .background() for exactly this query shape. Adding .background() here would move it onto the worker process.
2. The FTS subselect is unbounded. LocalSearchQueryBackend's visitMatch (app/src/services/search/search-query-backend-local.ts:270) emits id IN (SELECT content_id FROM ThreadSearch WHERE ThreadSearch MATCH ...) with no inner LIMIT — unlike SearchMatcher.whereSQL, which caps at 1000. So for a common token like support, SQLite materializes every matching thread id before the outer ORDER BY lastMessageSentTimestamp DESC LIMIT 100 is applied.
3. It doesn't actually restrict to people you've written to. lastMessageSentTimestamp > 0 means "this thread contains at least one sent message", and contactsMatchingEmailPrefix then scans thread.participants — everyone on the thread, including senders and anyone CC'd. So a support@ address that merely appeared alongside you on a thread will be suggested as if you'd emailed it.
Combined with the prefix filter below, the query casts a wide net (to_ : "support"* matches the domain too) and then discards nearly everything it fetched.
Generated by Claude Code
| const [_results, historyResults] = await Promise.all([query, historySearch]); | ||
| let results = this._distinctByEmail( | ||
| this._omitFindInMailDisabled(historyResults.concat(_results)) | ||
| ); | ||
| for (const ext of extensions) { | ||
| results = await ext.findAdditionalContacts(search, results); | ||
| } | ||
| results = prioritizeContactsMatchingEmailPrefix(this._distinctByEmail(results), search); | ||
| if (results.length > limit) { | ||
| results.length = limit; | ||
| } |
There was a problem hiding this comment.
These lines change the ranking of every autocomplete result, for everyone — which I think deserves to be an explicit product decision rather than a side effect of the fix.
Two compounding effects:
historyResults.concat(_results)puts thread-derived contacts first, and_distinctByEmailreturnsObject.values(uniq)in insertion order, so history wins over therefs DESCordering from the database query.prioritizeContactsMatchingEmailPrefixthen floats every local-part prefix match above everything else.
Then results.length = limit truncates to 5. So a contact you've emailed once, whose address happens to start with what you typed, can push out the contact you email daily. For example, typing sup would rank a rarely-used support@somevendor.com above a frequently-used Support Team <team@example.com>.
If prefix-boosting is desirable (it might well be!), I'd suggest making it a tiebreaker within the refs ordering rather than a hard partition — e.g. sort by (isPrefixMatch, refs) instead of concatenating two buckets.
Generated by Claude Code
| return results; | ||
| }) as any as Promise<Contact[]>; | ||
| const historySearch = this._searchSentRecipientContacts(search, limit).catch((err) => { | ||
| console.warn('Unable to search sent-recipient history for autocomplete', err); |
There was a problem hiding this comment.
Since this sits on the keystroke path, a failure here would log once per character typed. AppEnv.reportError is the convention elsewhere in the codebase for swallowed errors, and it de-dupes.
Generated by Claude Code
| ContactStore.searchContacts(input), | ||
| ]) | ||
| ).flat() | ||
| ).flat() as Contact[] |
There was a problem hiding this comment.
This cast isn't accurate — the array really does contain ContactGroup instances, since searchContactGroups results are flattened in alongside the contacts (and the p instanceof ContactGroup branch on line 73 depends on that). Asserting Contact[] here discards type information that the rest of the component relies on rather than fixing a type error.
If searchContacts becoming async broke inference here, (Contact | ContactGroup)[] would be the honest annotation.
Generated by Claude Code
| if (CustomComponent) return <CustomComponent token={p} />; | ||
| if (p instanceof Contact) { | ||
| return <Menu.NameEmailContent name={p.fullName()} email={p.email} key={p.id} />; | ||
| return <Menu.NameEmailContent name={p.fullName()} email={p.email} key={p.id || p.email} />; |
There was a problem hiding this comment.
Could you say what this fixes? I'd guess contacts synthesized from thread participants sometimes lack an id, in which case the fallback makes sense — but it'd be worth a short comment, or splitting it into its own commit, so it doesn't read as unrelated.
Generated by Claude Code
| prioritizeContactsMatchingEmailPrefix, | ||
| } from '../../src/flux/stores/contact-store'; | ||
|
|
||
| describe('contactSearchFetchLimit', () => { | ||
| it('still fetches contacts when no legacy account rows are loaded', () => { | ||
| expect(contactSearchFetchLimit(5, 0)).toBe(5); | ||
| }); | ||
|
|
||
| it('allows room to deduplicate contacts from multiple accounts', () => { |
There was a problem hiding this comment.
These assertions restate the one-line implementation of contactSearchFetchLimit rather than exercising the behavior that regressed, so they wouldn't catch a recurrence of the reported bug.
The ContactStore suite below is still xdescribe'd, so searchContacts itself has no coverage at all. If you're up for it, re-enabling that suite and adding a case for your scenario — a support@… contact that should be returned for the query support — would be far more valuable than the helper tests, and would give us a failing test to confirm the root cause against.
Generated by Claude Code
|
This pull request has been mentioned on Mailspring Community. There might be relevant details there: |
3 participants
Problem
When I typed in 'support', nothing showed up in the autocomplete list. I have multiple support emails that I email with regularly
Summary
Testing